Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

managing global configuration #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

managing global configuration #24

wants to merge 2 commits into from

Conversation

temanisparsh
Copy link
Contributor

@temanisparsh temanisparsh commented Jul 22, 2020 •
edited
Loading

Description of PR

Improve managing global configuration

Relevant Issues
Fixes #8

Checklist

  • Passes unit tests
  • Passes lint tests

Optional notes

In case the user has not created the config file, it throws an error saying:

WARNING: No configurations found in configuration directory:/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/config
WARNING: To disable this warning set SUPPRESS_NO_CONFIG_WARNING in the environment.

If the file is present but is empty, in that case it throws the following error:

/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/config/lib/config.js:182
    throw new Error('Configuration property "' + property + '" is not defined');
          ^
Error: Configuration property "app.auth" is not defined
    at Config.get (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/config/lib/config.js:182:11)
    at Object.<anonymous> (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/src/middleware/auth.middleware.ts:19:32)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Module.m._compile (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:858:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:861:12)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/src/routers/auth.router.ts:5:1)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Module.m._compile (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:858:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object

aniketnk
aniketnk requested changes Jul 23, 2020
View reviewed changes
Copy link
Collaborator

@aniketnk aniketnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Errors are super informative.
Resolve the comments, then we can merge.

Still looking into why the lint test is failing. @parthvshah Any thoughts?

.gitignore Outdated
@@ -42,7 +42,7 @@ node_modules/
jspm_packages/

# Configuration file
src/config.ts
config/default.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default.json exist by default?
The default flow should work without any modifications to the repo. And if the user wants to change the default mechanism, we will have to add in another json, and pass that to the config library (through env or any other mechanism)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will update the PR

@parthvshah
Copy link
Member

parthvshah commented Jul 23, 2020 •
edited
Loading

Still looking into why the lint test is failing. @parthvshah Any thoughts?

Files that have been modified in the commit(s):
File:[.gitignore], File_type:[gitignore]
  - WARN! Failed to get filetype for:[.gitignore]!
File:[README.md], File_type:[md]
File:[config/example.json], File_type:[json]
grep: config/example.json: No such file or directory
grep: config/example.json: No such file or directory
grep: config/example.json: No such file or directory
/action/lib/linter.sh: line 537: config/example.json: No such file or directory
/action/lib/linter.sh: line 556: config/example.json: No such file or directory

So I think this is the issue. Looks like the linter fails if you are trying to add a new file and it isn't present in the repo. This would explain the issue with PR #23 too.

Still looking into this.

@temanisparsh temanisparsh requested a review from aniketnk July 24, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@souravtecken souravtecken Awaiting requested review from souravtecken

@aniketnk aniketnk Awaiting requested review from aniketnk

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve managing global configuration
3 participants
@temanisparsh @parthvshah @aniketnk